Add neural network modules, tests, and test utils #33
Conversation
thisac
left a comment
thisac
left a comment
There was a problem hiding this comment.
Have yet to look at tests. Would this fit better in an nn folder, allowing more torch modules to be further organized into their own files. Can still be kept under the torch.nn namespace.
VolodyaCO
left a comment
VolodyaCO
left a comment
There was a problem hiding this comment.
Left a question about SkipLinear
anahitamansouri
left a comment
anahitamansouri
left a comment
There was a problem hiding this comment.
Thanks very much Kevin for the great work. I have left my comments.
|
RE @thisac : restructured the file into (sub)modules by pattern-matching pytorch's structure. |
kevinchern
requested review from
VolodyaCO,
anahitamansouri,
randomir and
thisac
anahitamansouri
left a comment
anahitamansouri
left a comment
There was a problem hiding this comment.
Thanks very much Kevin for addressing the comments.
thisac
left a comment
thisac
left a comment
There was a problem hiding this comment.
Thanks @kevinchern. I feel like the tests/utils and its tests could be somewhat simplified, and maybe cut down a bit, otherwise nothing major.
Co-authored-by: Theodor Isacsson <[email protected]>
|
RE @thisac : addressed most comments and left some clarifying questions in comments |
|
@thisac updated again based on your feedback on best practices. Thanks!!
|
Co-authored-by: Theodor Isacsson <[email protected]>
|
Can't find the specific comment about separating assertions into different test cases, but for posterity: I had a conversation with @thisac about the essence of "one assert per test case" and I am convinced that's the better practice (at least in this context). (No insight to share here. My disagreement came from inexperience 😝) |
5 participants
No description provided.